Skip to content

feat: quadratic + cubic sampling#3167

Closed
AlpinDale wants to merge 13 commits into
vllm-project:mainfrom
AlpinDale:feat/quadratic_sampling
Closed

feat: quadratic + cubic sampling#3167
AlpinDale wants to merge 13 commits into
vllm-project:mainfrom
AlpinDale:feat/quadratic_sampling

Conversation

@AlpinDale
Copy link
Copy Markdown
Contributor

This PR adds Quadratic and Cubic Sampling methods to vLLM.
Ref: oobabooga/textgen#5403 and oobabooga/textgen#5551

@simon-mo simon-mo requested a review from Yard1 March 3, 2024 22:58
Comment thread vllm/model_executor/sampling_metadata.py Outdated
Comment thread vllm/model_executor/layers/sampler.py
@Yard1
Copy link
Copy Markdown
Collaborator

Yard1 commented Mar 4, 2024

@AlpinDale Thanks, this looks good to me. Could we add a test?

@AlpinDale
Copy link
Copy Markdown
Contributor Author

Thanks for the review @Yard1. It needs a bit more work, could probably use a mask and an unsqueeze for the factor/curve. I'll finish this up soon and add a test unit.

Comment thread vllm/model_executor/sampling_metadata.py Outdated
@AlpinDale
Copy link
Copy Markdown
Contributor Author

@Yard1 Should be done, let me know if there's any issues. Correctness tests pass for the new sampler.

Comment thread vllm/model_executor/layers/sampler.py Outdated
Tested with no issues.

Co-authored-by: Antoni Baum <antoni.baum@protonmail.com>
Comment thread vllm/model_executor/layers/sampler.py Outdated
Comment thread vllm/model_executor/layers/sampler.py
Comment thread vllm/model_executor/layers/sampler.py Outdated
@Yard1
Copy link
Copy Markdown
Collaborator

Yard1 commented Mar 5, 2024

Hey @AlpinDale there seems to be an error in the test (the condition is not being met) that was hidden before as logits was modified in place. Could you take a look?

Also the condition itself is very brittle - it would be better to compare the output to some reference.

@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you!

@github-actions github-actions Bot added the stale Over 90 days of inactivity label Oct 30, 2024
@mergify mergify Bot added the frontend label Oct 30, 2024
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Oct 30, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. @AlpinDale please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Oct 30, 2024
@github-actions github-actions Bot added unstale Recieved activity after being labelled stale and removed stale Over 90 days of inactivity labels Nov 2, 2024
@hmellor
Copy link
Copy Markdown
Member

hmellor commented Feb 17, 2025

@AlpinDale do you plan to continue this line of work?

@AlpinDale
Copy link
Copy Markdown
Contributor Author

@AlpinDale do you plan to continue this line of work?

This sampler is quite outdated by now, so I'm not sure it'll be a useful addition anymore. There are better alternatives.

@hmellor
Copy link
Copy Markdown
Member

hmellor commented Feb 18, 2025

Ok, thank you for the update!

@hmellor hmellor closed this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend needs-rebase unstale Recieved activity after being labelled stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants